Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced partitioning customizations (COMPOSER-2355) #926

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Sep 12, 2024

This PR adds a new advanced partitioning customization to the blueprint that has the following structure:

{
  "customizations": {
    "partitioning": {
      "plain": {
        "filesystems": [
          {
            "mountpoint": "",
            "minsize": 0,
            "label": "",
            "fstype": ""
          }
        ],
        "lvm": {
          "volume-groups": [ # only one supported, but maybe we can support multiple in the future
            {
              "name": "",
              "logical-volumes": [ # similar to filesystems but with lvm stuff
                {
                  "mountpoint": "",
                  "minsize": 0,
                  "label": "",
                  "name": "", # lv name
                  "type": ""
                }
              ]
            }
          ]
        },
        "btrfs": {
          "volumes": [ # only one supported, but maybe we can support multiple in the future
            {
              "size": 0,
              "subvolumes": [
                {
                  "name": "",
                  "mountpoint": "",
                  "size": 0
                }
              ]
            }
          ]
        }
      }
    }
  }
}

The customization is currently enabled only in Fedora. I plan to add support and tests in RHEL and CentOS in a follow-up PR.

When the new customizations are used, the base partition table for the image type is ignored. Instead, a new "custom" partition table is constructed from scratch by iterating through the customizations and creating partitions, volume groups and logical volumes, and btrfs volumes and subvolumes as needed. Boot, EFI, and root partitions are created automatically to make the partition table valid and usable. Parameters such as boot mode and partition table type should be determined by the caller of the function, which should be based on the distro, architecture, and image type. This is, in a way, much simpler than our existing ensureLVM() and ensureBtrfs() functions, which needed to modify an existing partition table's structure in a way that preserved some parts and not others.

The custom partition table creation function (NewCustomPartitionTable()) supports all kinds of valid but perhaps unwanted configurations. For example, it's possible to create a partition table with both LVM volume groups and btrfs volumes, or multiples of each. The customizations however don't allow this, but the structure supports it (see above) in case we want to expand what we support in the future.

Customization validation happens in the blueprint. Invalid values such as duplicate mountpoints, empty btrfs subvolume names, etc are caught by a validator in the blueprint code itself.

What's next:

  • Enable the new customizations in RHEL and CentOS.
  • Add support for swap partitions and swap logical volumes.
    • I already started on this but removed it from the current PR because it was already getting too long.
  • Add support for LUKS encryption.
  • Integrate into osbuild-composer: I'd like to have this prepared before this PR is merged, which is why I'm keeping this as draft.

@achilleas-k achilleas-k force-pushed the more-partitioning branch 4 times, most recently from c9376dc to 296fa3e Compare September 12, 2024 16:05
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look (and then ran out of time) - this looks super nice so far, two tiny comments and I will continue later.

A meta-question - how will this interplay with the otk-gen-partition-table helper? it seems there is a lot of overlap and we will also need to add lvm/lbtrfs/luks there. Will it be unavoidable that we have two code paths that are doing this full the blueprint and for the otk yaml input or is there a way to share code (or do we do that already because it all maps to the same underlying primitives)?. I will try to understand this question myself a bit better and maybe it's too big to answer in detail in a PR in which case we can just do a OOB brainstorm and then put the summary in here.

pkg/disk/disk.go Show resolved Hide resolved
}
}

func mkESP(size uint64) Partition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for later) we should make a note that we use this helper in otk-gen-partition-table somehow, would be nice to consolidate this (similar to mkBIOSBoot)

@achilleas-k
Copy link
Member Author

achilleas-k commented Sep 13, 2024

A meta-question - how will this interplay with the otk-gen-partition-table helper?

I don't know, but I'll tell you how I'm thinking about this.

The existing filesystem customizations are too simple for the target use cases of otk. Back in the day, the only partition table customization we had was setting the size of the disk (which grew /), and partition tables were fixed. Then we got the addition of extra filesystems, which would be applied to the existing base partition table for each image type and "upgrade" the layout to LVM automatically for easier management. Then we got the concept of "partitioning modes" which affected how the partition table would be modified to accommodate the new filesystems (add them as plain, convert to LVM, or convert to btrfs). It's all a bit automatic and maybe hard to explain to users (people interacting through blueprints), but it fit the kind of high-level space where we imagine blueprints should live in.

What this PR does is move a lot more of the description of the partition table up from inside the image definitions into the blueprint. I think this move would have been a bit controversial a couple of years ago. We would generally try to keep the blueprints simple, the general philosophy being that the base image definitions are well tested, they're a great starting point for just about anyone, and the limited range of customizations is a feature that offers safety and stability. The general thinking was that the wider we spread the range of customizations the less we can test, the number of "invalid" configurations grow, and the weaker our guarantees of stability become. Note that this might just be me talking, I don't mean to project my ideas of how things were and are onto the entire team, even though I might be presenting it that way.

I think we're slowly realising that this has been holding us back, that many (most?) users want more control, and they don't mind the occasional footgun (though I suspect many who claim to want "lots of flexibility, with a chance of footgun" tend to underestimate the pain they're opening themselves up to).

What this all means for otk: Otk was conceived as the tool for distro maintainers. osbuild-mpp done right. It's easier to think about the level of customization and flexibility that otk should expose: pretty much anything goes. There's no regard for lack of safety, users of otk "should know what they're doing". My first thought when I read your question was that the level of flexibility exposed by these customizations can be the same for the otk tooling. That seems strange to me though because blueprints should be more abstract/high-level than the distro maintainer tooling. It certainly seems (to me right now) that we could have blueprint customizations for partitioning that are powerful enough for a distro maintainer using otk, but I am worried about the requirements of otk pushing for more advanced features in blueprints. For example, as I see it, there should be no way using blueprints in osbuild-composer to build an image configured for "hybrid boot" without a BIOS boot partition. It should, however, be possible to draw up whatever partition table one wants using otk, because otk shouldn't be that smart.

So where does otk fit in the space between blueprint partitioning and make your own partition table to the dot? It's certainly annoying to have to write out a whole PT, but isn't that what otk is for? I'm actually not sure.

Examining a few answers

The otk external tooling should work exactly like blueprints (take in blueprint.customizations.partitioning and generate a full PT) and otk users that want more should write everything from scratch.

This doesn't really work though because the "from scratch" option would have no tooling for things like sector alignment and UUID generation, which would be a PITA.

The otk external should remain as it is (Partitions + Modifications).

This solution kinda sucks because it actually seems less expressive than the new partitioning customizations right now, which is backwards.

The otk external should work like blueprints + extra stuff (i.e. import blueprint.customizations.partitioning and expand the config objects with more options/fields, like fstab fields as well as some high level bits like BootMode for automatic partition creation).

Probably the best solution and close to what we have now, but needs a bit of work.

The otk external should take in a PT object (disk.PartitionTable) and fill in any gaps.

I kinda like this idea. It means that the new customizations in this PR have no relation to otk at all, but it gives flexibility to a distro maintainer to do whatever while getting some help from the disk module for proper PT and osbuild stage generation. The way I imagine this is that otk-gen-partition-table inputs should look like a disk.PartitionTable, with the option to add UUIDs, sizes, even start sectors, and anything NOT set gets filled in by the external. So you could, for example, have 3 partitions with filesystems, fill in the UUIDs for 2 of them, and the third gets an automatic/random UUID.
Same for things like mount options. An empty string implies defaults, but the field is exposed in the input and can be overridden.

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! I love it! I don't see any functional issues, just some minor things that I noticed.

Regarding OTK: The current plan is that osbuild-composer will switch to an OTK backend (through images) at some point, right? Then, we will need to somehow pass blueprints to OTK: So either we will have to translate both partitioning customizations to a unified format, or omnifests (and related externals) will simply have to understand both.

pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
pkg/blueprint/filesystem_customizations.go Show resolved Hide resolved
pkg/disk/partition_table.go Outdated Show resolved Hide resolved
pkg/disk/partition_table.go Show resolved Hide resolved
pkg/disk/partition_table.go Outdated Show resolved Hide resolved
pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
//
// There should always be one value for each filesystem type supported by
// osbuild stages (stages/org.osbuild.mkfs.*) and the unset/none value.
type FSType uint64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making this a string alias, and use descriptive values? Should make debugging a tad bit simpler. Are there downsides (apart from slightly worse performance)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not much downside other than making it "easy" to use strings in place of the actual value sometimes, which can make it hard to change or track. Having the enum backed by a uint says, a bit more clearly, "the underlying value doesn't matter", which I find encourages better use of the enum.

But I can go either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, I had the same thought and tried it out and it was the same about the amount of code/complexity, golang just does not have a good way of doing this it feels so I did not comment (maybe I should have).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any strong feelings either way? Should I go with strings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't have strong feelings. :)

pkg/disk/partition_table.go Show resolved Hide resolved
pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
@achilleas-k achilleas-k changed the title Advanced partitioning customizations Advanced partitioning customizations (COMPOSER-2355) Sep 17, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick comments on the first part, did not manage to get further yet, will try later today.

pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
}

// collect all mountpoints
mountpoints := make([]string, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(super nitpick) this could be just var mountpoints []string but probably just personal preference :)

@@ -24,6 +162,24 @@ func (fsc *FilesystemCustomization) UnmarshalTOML(data interface{}) error {
return fmt.Errorf("TOML unmarshal: mountpoint must be string, got %v of type %T", d["mountpoint"], d["mountpoint"])
}

switch d["type"].(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't test this right now? It's fine if we don't, I would love to have a test for this and also one that checks that the json strings are what we expect but I don't want this PR to become bigger, I can offer to do it as a followup :)

Copy link
Member Author

@achilleas-k achilleas-k Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to existing tests.

pkg/blueprint/filesystem_customizations.go Outdated Show resolved Hide resolved
//
// There should always be one value for each filesystem type supported by
// osbuild stages (stages/org.osbuild.mkfs.*) and the unset/none value.
type FSType uint64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, I had the same thought and tried it out and it was the same about the amount of code/complexity, golang just does not have a good way of doing this it feels so I did not comment (maybe I should have).

@achilleas-k
Copy link
Member Author

achilleas-k commented Sep 18, 2024

Fixed up the following from comments:

  • Moved required sizes out of the function. Now nil means no requirements. Added the defaults to all the Fedora image types except the iot-raw-image because that had required sizes explicitly disabled.
    • Updated tests.
  • All new JSON tags are snake_case.
  • Mountpoint validation: Previously the validator only checked for empty strings. Now it also makes sure all mountpoints are absolute paths and "clean" (filepath.Clean(path) == path).
    • Extra tests added.
  • Replaced all occurrences of MebiByte and GibiByte in TestNewCustomPartitionTable() with MiB and GiB because it looks nicer.
    • This one was just for me.

There are a couple of more comments still open.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is very nice and will be a very useful feature for our users. I look forward seeing it used (in e.g. bib). I added a bunch of comments/ideas etc but all optional.

What is missing to get it out of draft? I also wonder if might make sense to split into smaller chunks as reviewing 2k lines is not easy but then it's hard to split I guess.

Happy to approve fwiw

switch options.DefaultFSType {
case FS_EXT4, FS_XFS:
bootType = options.DefaultFSType
case FS_NONE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) AFAICT this check is orthogonal to the bootType selection, right? If so I personally would move the check for FS_NONE earlier as I was a bit confused by the two things happening here that are (seemingly?) othogonal (but maybe it's just me)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, NONE should be allowed. I think we should make it so that options.DefaultFSType == FS_NONE is only an issue when a non-boot partition is automatically created.

if _, err := payload.CreateLogicalVolume("", 0, rootfs); err != nil {
return nil, fmt.Errorf("error creating root logical volume: %w", err)
}
break PartitionLoop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(niptick) moving this into a helper function probably allows to avoid the "break PartitonLoop" here as this would just becomea "return" but I'm not sure how much of a win it would be (have not tried it)

rootpart := Partition{
Type: FilesystemDataGUID,
Bootable: false,
Size: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small comment like // 0 means auto-adjust or something might be nice, alternatively we could define a const PART_SIZE_AUTOADJUST 0 or something to allow conveying this without a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. 0 means autoadjust only in the case of / and even then it might stay 0 if there are no required sizes specified. I think a comment is the right way to go instead of a const that could imply more.

// If no separate requiredSizes are given then we use our defaults
if requiredSizes == nil {
requiredSizes = map[string]uint64{
"/": 1073741824,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) maybe common.GibiByte ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gone now and defined on Fedora image types, with GiB.

}

testCases := map[string]testCase{
"null": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call this "dos" or "hybrid-dos" here as name or something as a real "null" would leave PartitionTableType/BootMode blank?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

test/config-map.json Show resolved Hide resolved
pkg/distro/fedora/imagetype.go Show resolved Hide resolved
test/configs/partitioning-btrfs.json Show resolved Hide resolved
func NewCustomPartitionTable(customizations *blueprint.PartitioningCustomization, options *CustomPartitionTableOptions, rng *rand.Rand) (*PartitionTable, error) {
pt := &PartitionTable{}

if options == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(super nitpick) many projects (in my small sample that I have seen) put this check as the very first thing as a convention to never accidentally use options before it's initialized. Here it's almost the first and there is no risk so fine to ignore but I wanted to mention it because I like the convention(super nitpick) many projects (in my small sample that I have seen) put this check as the very first thing as a convention to never accidentally use options before it's initialized. Here it's almost the first and there is no risk so fine to ignore but I wanted to mention it because I like the convention

}

// NewCustomPartitionTable creates a partition table based almost entirely on the partitioning customizations from a blueprint.
func NewCustomPartitionTable(customizations *blueprint.PartitioningCustomization, options *CustomPartitionTableOptions, rng *rand.Rand) (*PartitionTable, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) This is a long function. It's okay because it is very linear so it's not a big deal, but I was wondering if it might be split up so that parts can be reused for otk and also if it would be easier to follow/extend with splitting (and test individually). I did a quick experiment in acb3825#diff-ffc14a35d85eb6fd9724427b51786e40bc3e910bc41cbd6762ab977ae9689765R455 which I kinda like but maybe just personal preferences - can of course be a followup

- New structure that's more powerful than the existing
  FilesystemCustomization.
- The general structure starts at 3 top-level objects:
    - Plain: customizations for plain disk partitioning, meaning
      partitions with traditional filesystem payloads (xfs, ext4, etc).
    - LVM: customizations for creating LVM layouts.  Supports one or
      more VolumeGroups.  Each VolumeGroup implies the creation of a
      partition with the appropriate type and payload.
    - Btrfs: customizations for creating btrfs layouts.  Supports one or
      more volumes and subvolumes for each volume.  Each btrfs volume
      implies the creation of a partition with the appropriate type and
      payload.
- Partitioning.Filesystems is an array of the old
  FilesystemCustomization struct.
- The new FilesystemCustomization struct supports a label and Type.
- The LVCustomization (logical volume) embeds the
  FilesystemCustomization and adds a name for the LV.

Unmarshal tests have been updated with new fields.
New function that creates a partition table from scratch based on the
new partitioning customizations.
Add one test for each general layout: plain, lvm, and btrfs.
Move the handling of the /boot partition before the plain partition
handling so we don't have to find where to insert it (after BIOS boot
and ESP, which are going to be added in a later commit, but before
plain partitions).  Instead, we build the partition list sequentially as
needed.

Update tests.
Move the BootMode enum from pkg/distro to pkg/platform.  We want to
import BootMode to the disk package, which would create an import loop
with distro.
Add BIOS boot and ESP partitions based on boot mode.  The boot mode is
passed as an argument to the NewCustomPartitionTable() function.

Update tests.
At the end of the partition table's creation, add a root partition if
one wasn't created during the processing of the customizations.
When the customizations argument is nil, initialise it with an empty
struct and continue processing the function normally so that the root
partition is created as well.
Add minsize to the top level of the partitioning customizations.
In osbuild-composer, this value comes from a command line option through
distro.ImageOptions.  The plan is to have that be a blueprint
customization that osbuild-composer will resolve when it's defined in
multiple places and pass it to images via the customization.
Add a new argument, defaultType, to the NewCustomPartitionTable()
function.  The purpose of the type is to determine which filesystem will
be used for root and boot when they are added automatically.

It will also be used to set the filesystem for any mountpoint that has
no type defined.

The default type should be determined by the distro definition.
When a custom partition or LogicalVolume does not specify a type, use
the defaultType.
Add a new enum, FSType, for referring to all supported filesystem types.
Use it as the default type argument to the NewCustomPartitionTable()
function.
Run a relayout() on the partition table before returning it from
NewCustomPartitionTable() to set the start position and size for each
partition as well as the total partition table size.

Update tests.

Partition sizes in tests changed to MiB (from plain bytes) so that the
values make more sense when headers are added and values are rounded to
the grain size (1 MiB).
@achilleas-k achilleas-k marked this pull request as ready for review September 19, 2024 18:08
ondrejbudai
ondrejbudai previously approved these changes Sep 20, 2024
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tiny documentation issue, but we can resolve it in a follow-up. Otherwise, this LGTM. Thanks a ton! ❤️

pkg/disk/partition_table.go Outdated Show resolved Hide resolved
Introduce the concept of required sizes from the existing
NewPartitionTable() function into the NewCustomPartitionTable().

RequiredSizes is necessary for setting a "sane default" (minimum) size
for certain directories.  Unlike the old partition table creation
function, there is no default value for the custom partition table
generator.  Instead, image definitions will need to set their own
defaults.

Without this mechanism, automatically created root filesystems can end
up being too small to fit the base OS packages or to have a usable
system in the long run.  It's also needed when automatically creating
root partitions or LVM Logical Volumes.  Without a required minimum, the
root partition will only be the size of the leftover space on the image
(the base image size minus the sum of the rest of the partitions, which
can sometimes end up being 0) or LVM VolumeGroup.

Add some required sizes to tests.
This is leftover from the repo split.  It is implemented and used in
osbuild-composer.
This is the equivalent to the CheckMountpointsPolicy() function for the
new PartitioningCustomization.  It collects all the mountpoints from
Plain, LVM, and Btrfs then compares them to the policy.
Preparing the function to use the new PartitioningCustomization when
set.
For all image types that don't specify a requiredPartitionSizes, add the
defaults that are used in the NewPartitionTable() so that
NewCustomPartitionTable() has similar behaviour.

The only image type that explicitly doesn't specify minimum sizes is the
iot-raw-image, which has a pre-existing empty map.
Add checks for Partitioning customizations in Fedora's checkOptions()
function.  The function now returns an error if:
- Partitioning is defined at the same time as Filesystem
- Partitioning is defined for ostree image types (commit and container).
- The mountpoints in Partitioning violate the mountpoint policies for
  the image type.
Add a Validate() method to PartitioningCustomization that returns an
error if:
1. Btrfs and LVM are both defined.
2. Any mountpoint is invalid (empty string, relative path, or not
   "clean").
3. Any mountpoint is defined more than once in the whole structure.
4. There is more than one LVM Volume Group or more than one btrfs
   volume.
5. Any LVM Logical Volume name is used more than once in the same
   VolumeGroup.
6. Any Btrfs subvolume has an empty name.
7. Any Btrfs subvolume name is used more than once in the same Btrfs
   volume.

This function validates the customization itself in the absence of any
distro or image-type specific policies.

We may relax rule 3 in the future, at least in images if not in
osbuild-composer, but for now, let's disallow it while keeping the
customization structure flexible.

val
The VolumeGroup.CreateLogicalVolume() function assumed that the given
name was a mountpoint and would automatically escape it, append 'lv',
and append numbers in case of collisions.  This made it impossible to
use the helper function for LV creation with desired names.

Refactor the function and modify all the calls so that now it uses the
provided name without validating, escaping, or checking for collisions,
and generates a name from the payload's mountpoint when one is not
provided.

The CreateMountpoint() function's functionality is not affected.
Use the CreateLogicalVolume() helper when creating LVs for the custom
partition table.  This changes size calculations in tests because each
LV is resized to be an integer number of extents, i.e. sizes are rounded
up to 4 MiB multiples.
When a volume group's name is not specified in the customizations, the
custom partition table creator assigned the names vg00, vg01, etc.

A new test is added with multiple volume groups to test this.  The
scenario of the test is not supported by customizations, the blueprint
package does not allow multiple volume groups, but the function can
handle it so it's good to have it covered.
One for each variant:
- Plain
- Plain + btrfs
- Plain + lvm
Added extra validation for:
- invalid filesystems for specific mountpoints (e.g. xfs on /boot/efi)
- /boot or /boot/efi on lvm or btrfs
- btrfs as a filesystem on plain

Added tests to cover all new validation errors.
Added an extra happy test for /boot and /boot/efi with automatic fs type
selection (unset, empty strings).
A small function for resolving the filesystem type for a mountpoint with
the global default.  Will return an error if both are unset.
Split the large NewCustomPartitionTable() function into smaller ones for
readability and nicer handling of error conditions (no need to beak on a
loop label).
- Legacy boot mode.
- UEFI boot mode.
- Auto root creation on btrfs.
Change the error message for invalid partition table types to match the
other error messages from the NewCustomPartitionTable() function.

Add a test.
ondrejbudai
ondrejbudai previously approved these changes Sep 20, 2024
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are fast! 😍

@ondrejbudai ondrejbudai dismissed their stale review September 23, 2024 13:55

I need one more day to read through user feedback.

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme go over user feedback that I just got before merging it. :)

@@ -188,6 +188,9 @@ func mkESP(size uint64) Partition {
}

type CustomPartitionTableOptions struct {
// PartitionTableType must be either "dos" or "gpt". Defaults to "gpt".
PartitionTableType string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an enum-like type for filesystem types, I think it would make sense to be consistent and do the same here instead of strings.

// requirements are additive, meaning the minimum size for a mountpoint's
// partition is the sum of all the required directory sizes it will
// contain.
RequiredSizes map[string]uint64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I'd call it RequiredMinSizes or maybe reuse FilesystemCustomization so that it's clear just from the variable name that these are minimums and not an exact size (even though the docstring clearly explains that).

@bcl
Copy link
Contributor

bcl commented Sep 24, 2024

Wow, great job with this! I only had a couple small comments and one thing I didn't see -- is there anything that catches overrlapping partitions? I didn't notice anything in the validation function or tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants